-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[libc++] Ensure that we restore invariants in basic_filebuf::overflow #147389
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
In rare circumstances, the invariants could fail to be restored.
@llvm/pr-subscribers-libcxx Author: Louis Dionne (ldionne) ChangesIn rare circumstances, the invariants could fail to be restored. Full diff: https://github.com/llvm/llvm-project/pull/147389.diff 1 Files Affected:
diff --git a/libcxx/include/fstream b/libcxx/include/fstream
index c86f709bedb80..283e31294cc73 100644
--- a/libcxx/include/fstream
+++ b/libcxx/include/fstream
@@ -821,6 +821,14 @@ typename basic_filebuf<_CharT, _Traits>::int_type basic_filebuf<_CharT, _Traits>
template <class _CharT, class _Traits>
typename basic_filebuf<_CharT, _Traits>::int_type basic_filebuf<_CharT, _Traits>::overflow(int_type __c) {
+ auto __failure = [this] {
+ if (this->pptr() == this->epptr() + 1) {
+ this->pbump(-1); // lose the character we overflowed above -- we don't really have a
+ // choice since we couldn't commit the contents of the put area
+ }
+ return traits_type::eof();
+ };
+
if (__file_ == nullptr)
return traits_type::eof();
__write_mode();
@@ -841,8 +849,9 @@ typename basic_filebuf<_CharT, _Traits>::int_type basic_filebuf<_CharT, _Traits>
if (__always_noconv_) {
size_t __n = static_cast<size_t>(this->pptr() - this->pbase());
- if (std::fwrite(this->pbase(), sizeof(char_type), __n, __file_) != __n)
- return traits_type::eof();
+ if (std::fwrite(this->pbase(), sizeof(char_type), __n, __file_) != __n) {
+ return __failure();
+ }
} else {
if (!__cv_)
std::__throw_bad_cast();
@@ -854,34 +863,38 @@ typename basic_filebuf<_CharT, _Traits>::int_type basic_filebuf<_CharT, _Traits>
char* __extbuf_end = __extbuf_;
do {
codecvt_base::result __r = __cv_->out(__st_, __b, __p, __end, __extbuf_, __extbuf_ + __ebs_, __extbuf_end);
- if (__end == __b)
- return traits_type::eof();
+ if (__end == __b) {
+ return __failure();
+ }
// No conversion needed: output characters directly to the file, done.
if (__r == codecvt_base::noconv) {
size_t __n = static_cast<size_t>(__p - __b);
- if (std::fwrite(__b, 1, __n, __file_) != __n)
- return traits_type::eof();
+ if (std::fwrite(__b, 1, __n, __file_) != __n) {
+ return __failure();
+ }
break;
// Conversion successful: output the converted characters to the file, done.
} else if (__r == codecvt_base::ok) {
size_t __n = static_cast<size_t>(__extbuf_end - __extbuf_);
- if (std::fwrite(__extbuf_, 1, __n, __file_) != __n)
- return traits_type::eof();
+ if (std::fwrite(__extbuf_, 1, __n, __file_) != __n) {
+ return __failure();
+ }
break;
// Conversion partially successful: output converted characters to the file and repeat with the
// remaining characters.
} else if (__r == codecvt_base::partial) {
size_t __n = static_cast<size_t>(__extbuf_end - __extbuf_);
- if (std::fwrite(__extbuf_, 1, __n, __file_) != __n)
- return traits_type::eof();
+ if (std::fwrite(__extbuf_, 1, __n, __file_) != __n) {
+ return __failure();
+ }
__b = const_cast<char_type*>(__end);
continue;
} else {
- return traits_type::eof();
+ return __failure();
}
} while (true);
}
|
libcxx/test/std/input.output/file.streams/fstreams/filebuf.virtuals/overflow.writefail.pass.cpp
Show resolved
Hide resolved
@@ -841,8 +849,9 @@ typename basic_filebuf<_CharT, _Traits>::int_type basic_filebuf<_CharT, _Traits> | |||
|
|||
if (__always_noconv_) { | |||
size_t __n = static_cast<size_t>(this->pptr() - this->pbase()); | |||
if (std::fwrite(this->pbase(), sizeof(char_type), __n, __file_) != __n) | |||
return traits_type::eof(); | |||
if (std::fwrite(this->pbase(), sizeof(char_type), __n, __file_) != __n) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC you're currently only testing this path. We should also test the fwrite
calls in the else
path (i.e. have a locale that's not __always_noconv_
and have that return the different possible states).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The facet for wchar_t
returns always_noconv() == false
, so we're also testing below. We could try to hit every branch in the if
but I think there is diminishing returns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK with merging this as-is into main, but I do think we should look into improving our test coverage (in general). AFAICT we have not even a single test that actually tests all the branches.
/cherry-pick 6291b63 |
…149390) This patch makes the `__failed` lambda a member function on `fstream`. This fixes two LLDB expression evaluation test failures that got introduced with #147389: ``` 16:22:51 ******************** 16:22:51 Unresolved Tests (2): 16:22:51 lldb-api :: commands/expression/import-std-module/list-dbg-info-content/TestDbgInfoContentListFromStdModule.py 16:22:51 lldb-api :: commands/expression/import-std-module/list/TestListFromStdModule.py ``` The expression evaluator is asserting in the Clang parser: ``` Assertion failed: (capture_size() == Class->capture_size() && "Wrong number of captures"), function LambdaExpr, file ExprCXX.cpp, line 1277. PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace. ``` Ideally we'd figure out why LLDB is falling over on this lambda. But to unblock CI for now, make this a member function. In the long run we should figure out the LLDB bug here so libc++ doesn't need to care about whether it uses lambdas like this or not.
…r function (#149390) This patch makes the `__failed` lambda a member function on `fstream`. This fixes two LLDB expression evaluation test failures that got introduced with llvm/llvm-project#147389: ``` 16:22:51 ******************** 16:22:51 Unresolved Tests (2): 16:22:51 lldb-api :: commands/expression/import-std-module/list-dbg-info-content/TestDbgInfoContentListFromStdModule.py 16:22:51 lldb-api :: commands/expression/import-std-module/list/TestListFromStdModule.py ``` The expression evaluator is asserting in the Clang parser: ``` Assertion failed: (capture_size() == Class->capture_size() && "Wrong number of captures"), function LambdaExpr, file ExprCXX.cpp, line 1277. PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace. ``` Ideally we'd figure out why LLDB is falling over on this lambda. But to unblock CI for now, make this a member function. In the long run we should figure out the LLDB bug here so libc++ doesn't need to care about whether it uses lambdas like this or not.
/cherry-pick 6291b63 |
/pull-request #155712 |
In rare circumstances, the invariants could fail to be restored.